-
Notifications
You must be signed in to change notification settings - Fork 107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check tbsCertificate signature algorithm matches certificate #436
Conversation
Per RFC 5280 section 4.1.1.2, couldn't find an existing lint.
Just to add a little context after some discussions elsewhere, this lint is probably of value to a CA that is using zlint, since it may catch creating a certificate that is somewhat... dubious post signature, but is likely of significantly less value as a lint for larger corpora, like CT, since the certificate signatureAlgorithm field is not covered by the signature and is as such malleable. This means anyone can submit a bunch of doppelgänger certificates with an altered algorithmIdentifier sequence which would trigger this lint (an example being the 10 certs caught using the corpus integration tests, which all appear to be altered versions of their original certificates. I'd probably be find abandoning this change given this... |
Hey @rolandshoemaker! Thanks for the PR. I haven't had a chance to review it yet but wanted to leave a comment.
I think it's worth keeping if the tradeoffs you mention are documented (in code comments or elsewhere). We actually have an outstanding issue for this lint that @sleevi filed: #419 I think we were favouring an approach that would put more of the ASN.1 work into ZCrypto (zmap/zcrypto#213) WDYT? |
Ah interesting, I completely missed that issue. In terms of exposing the signature that seems viable, but I'm not sure it's what you'd entirely want for this particular lint. Using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rolandshoemaker, this is a nice clean implementation ✨
Can you take a pass at fixing the integration test data based on the docs here? It looks like there's also a ~10 error level findings in the corpus. If you could give those a quick look-over when you update the integration/config.json
to make sure they're true-positives that would be great.
For my review I flagged a few nits but there's nothing worth holding this PR up on. I'd like to give @sleevi a chance to 🔍 this before I merge but if he's tied up I'm also comfortable pulling the trigger once CI passes and letting it bake in-tree before the next RC.
Thanks again!
v2/lints/rfc/lint_tbs_signature_alg_matches_cert_signature_alg.go
Outdated
Show resolved
Hide resolved
v2/lints/rfc/lint_tbs_signature_alg_matches_cert_signature_alg.go
Outdated
Show resolved
Hide resolved
Thanks for the reviews, think I hit everything. |
Per RFC 5280 section 4.1.1.2, couldn't find an existing lint.
Resolves #419